Repository navigation
Bulk import boxes: imported-box tasks only work on their own import's boxes - #1858
Conversation
The extract-then-identify intake and the per-box extraction children added for bulk imports with user-supplied boxes now check what they are handed before changing anything: - IA.intakeImportedBoxes stages an annotation only when its feature carries the stamp of the import named on the request's root task (Annotation.isImportedBoxOf, also used by the envelope builder). Anything else is logged and left untouched; the recovery holder applies the same rule. - MlServiceProcessor.process: an imported-box child accepts only a job that names the one box in the job the server stored on it, and that box must carry the child's import stamp. Any other job is dropped before a write. The check uses strict lookups, so a datastore failure or unreadable stored task parameters return an error rather than running the job. - IAGateway: an imported-box child is never reused as a request's root, and the imported-box marker is removed from incoming task parameters on both the enqueue and the bulk-import hand-off paths. Reviewed with Codex over five rounds. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1858 +/- ##
=======================================
Coverage 59.43% 59.43%
=======================================
Files 320 320
Lines 13104 13104
Branches 4274 4274
=======================================
Hits 7788 7788
Misses 5033 5033
Partials 283 283
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
UrbanFercec
left a comment
UrbanFercec
left a comment
There was a problem hiding this comment.
Ran it with #1860 on top as well, 279 tests green including the DbTests. Three things inline, the first one also hits ordinary detection jobs.
| try { | ||
| mismatch = importedBoxJobMismatch(taskId, named); | ||
| } catch (Exception ex) { | ||
| return MlServiceJobOutcome.persistError("PERSIST", "imported-box check failed: " + ex); |
There was a problem hiding this comment.
This catch covers every job with a taskId, not only imported boxes. I ran a plain task with parameters {broken and a mediaAssetId job: ERROR_PERSIST, no ML call, task stays initiated, asset stays processing. On main the same job completes. Failing closed only when the raw parameters carry the marker, or calling markDetectionFailure here like the load path does, would avoid that.
There was a problem hiding this comment.
Thanks, good catch. Fixed in f413547: an unreadable-parameters task now fails closed only when something persisted says it could be an imported-box child (the marker text, a stored job that names an annotation, or a held imported box). A plain task with {broken parameters and a mediaAssetId job passes the check and runs as on main. New test unreadableParametersWithoutTheMarkerDoNotStopAnOrdinaryJob; the existing corruption test still covers a real imported-box child (including none, Unknown and {broken).
| tparams.put("username", tuser.getUsername()); | ||
| } | ||
| // the imported-box marker is only ever set by the server, on the children it creates | ||
| tparams.remove(Task.PARAM_IMPORTED_BOX); |
There was a problem hiding this comment.
This drops only the top-level key, but ImportedBoxReplay matches "importedBox":true anywhere in the stored parameters. Through gatewayTaskParams, {"x":{"importedBox":true}} is kept and the task gets replayed on every restart, and enough of them fill the 500 oldest slots. Re-checking Task.isImportedBoxTask(t) in fetchStaleResumeMessages would close it.
There was a problem hiding this comment.
Fixed in f413547. The gateway now removes the marker at every depth of the task parameters (both gatewayTaskParams and handleBulkImport), and fetchStaleResumeMessages re-checks Task.isImportedBoxTask on each row. Candidates are read in pages, so rows that don't qualify never count toward the 500, and the rows examined per run are strictly bounded. Tests: gatewayTaskParamsDropTheMarkerAtEveryDepth, aNestedMarkerIsNotAnImportedBoxChild (including limit 1 with an older non-qualifying row), theScanBoundIsStrict.
| " no longer exists; skipped"); | ||
| continue; | ||
| } | ||
| if (!ann.isImportedBoxOf(importId)) { |
There was a problem hiding this comment.
When every box gets skipped here, nothing marks the root, so a client polling it sees a task that never finishes. rootWithoutAnImportStampExtractsNothing checks that nothing ran but not the root's status. Maybe mark the root as an invalid envelope when boxes were named and none were taken?
There was a problem hiding this comment.
Fixed in f413547. When no named box is taken, the root now ends: error / INVALID_ENVELOPE if a named annotation exists but isn't a box of this import, completed if they were all deleted. A failed commit of that end goes through the existing staging recovery (root ends EXTRACTION_PERSIST). rootWithoutAnImportStampExtractsNothing now checks the root status; new tests rootWhoseBoxesAreAllGoneCompletesWithNothingToDo and aFailedCommitOfTheEmptyRootEndsItThroughRecovery. The import page's summary does not yet read this request's end for the all-deleted case; that is a follow-up on #1861, which owns the boxed-import completion rule.
The submissions hand-off tests that arrived with #1851 mock an annotation. The hand-off now selects an import's boxes through isImportedBoxOf, so the mock has to answer it; without the stub the mocked box was left out and two tests failed once main was merged in. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
From UrbanFercec's review of #1858: - A task whose stored parameters do not parse no longer stops an ordinary job. The imported-box check fails closed only when something persisted says the task could be an imported-box child: the marker text, a stored job that names an annotation, or a held imported box. Detection jobs on such tasks run as before. - The gateway removes the imported-box marker at every depth of client-supplied task parameters, and startup replay republishes only tasks whose parsed top-level flag is set. Candidates are read in pages, so rows that do not qualify never count toward the per-run limit, and the number of rows examined is strictly bounded. - An extraction request whose named boxes were all skipped now ends: in error (INVALID_ENVELOPE) when a named annotation belongs to another import, completed when they were all deleted. A failed commit of that end goes through the existing staging recovery. Reviewed with Codex over three rounds (converged). Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @UrbanFercec! Lemme know of the fixes address your concerns. |
Follow-up to #1848 (bulk import boxes A), which merged before review was finished.
What
The extract-then-identify intake and the per-box extraction tasks added for bulk imports with user-supplied boxes now check what they are handed before changing anything. They only ever work on a box that the named import created.
How
IA.intakeImportedBoxesstages an annotation only when its feature carries the stamp of the import named on the request's root task. Anything else is logged and left untouched (no task, no status change). The recovery holder applies the same rule.Annotation.isImportedBoxOf(importTaskId), used by the intake, the envelope builder (IAGateway.importedBoxAnnotationIds) and the processor.MlServiceProcessor.processchecks every job for an imported-box child: it must name the one box in the job the server stored on that child, and the box must carry the child's import stamp. Any other job is dropped before a write. The check uses strict lookups, so a datastore failure or unreadable stored task parameters (Task.hasUnreadableParameters) return an error and the job does not run.No change for imports without boxes, for normal detection and identification, or for legitimate boxed imports: the importer already stamps every box it creates, and the server-built jobs (intake, startup replay, deferred matches, retries) all pass the check.
Testing
ImportedBoxIntakeDbTest,MlServiceProcessorImportedBoxDbTest,IAGatewayRequestRootTestandIAGatewayImportEnvelopeTest: annotations that are not this import's boxes are left untouched (identification status, tasks and the asset's detection status all unchanged); jobs of every other shape for an imported-box child are dropped; a failed lookup or unreadable parameters is an error, not a pass; server-published deferred matches still run.Not verified: the full suite was not run locally (CI will), and nothing was exercised on a running install.
Reviewed with Codex over five rounds until it reported no remaining findings.
🤖 Generated with Claude Code